I merely think people will combine the methods as such

!python -m pip install git+https://github.com/joelostblom/altair.git@method-based-attr-setting

import altair as alt
from vega_datasets import data

source = data.cars.url

s1 = alt.Scale(zero=False)
s2 = alt.Scale(scheme='viridis')

chart = alt.Chart(source).mark_point().encode(
    x=alt.X('Horsepower:Q').axis(labels=False).scale(s1).title('foo'),
    color=alt.Color('Origin:N', scale=s2).legend(orient='bottom')
)
chart

Observe on the x encoding, I use the .scale(s1) and on the color encoding I use the scale=s2. It still works though.

Personally I think this deep nested setting and getting of attributes is neither a show-stopper nor a big issue. I do not think it is common usage by normal users. What I did do in the past is when a newer version of VL can render something which cannot yet pass validation in altair that I compile the chart into a VL-dict and then change the value of the corresponding key. Something as such:

ctd = chart.to_dict()
ctd['encoding']['x']['title'] = {'signal': 'bah'}

But it felt like a hack and not following the zen of altair, but again, this will still works.

I think the interesting outstanding issue is the part how we can solve this keyword argument completion for these methods.

I made some progress on the completions in my latest commit! The help now shows the correct signature and docstring:

(there are some unnecessary fields here that should be removed somehow, type, file, etc)

The tab completion works for the class (the suboptimal ordering is an issue with jlab/ipython, but the maxbins param shows up):

but not for instances of the class:

I asked about this on SO, but if anyone here has ideas, please chime in. Update: The SO question is solved but I think there is a separate issue in Altair that still prevents params from showing.

One last issue I am running into is that completion for the method name also only works for the class, but not for instances of the class. So typing alt.X.b and then pressing Tab completes to bin but there is no completion on alt.X().b (this works in a toy example so I think it is an altair specific issue)

Hi @joelostblom, is there anything I can do to help with this PR? I think it would be great if this syntax can be used after the next major release.

Thank you! It would be awesome if you want to look into getting the tab completion to work properly! Both the method name and the parameters inside the method are currently not showing up in the tab completion. I gave a detailed example on exactly what works and what doesn't in my comment just above and I have not worked on it more since then. I think that is the only technical problem left to fix (then there is this potential conceptual challenge #1629 (comment) and we need to add some tests and docs, but that's later). Let me know if it is unclear and I will try to clarify.

Sorry for my git ignorance @joelostblom... should I be able to switch between my own branches of Altair and yours here: https://github.com/joelostblom/altair/tree/method-based-attr-setting
I haven't been able to figure out how to reach your method-based-attr-setting on my own computer. Should I just create a whole new folder on my computer for your branch?

All but the basics of git tend to leave me confused...

No worries Chris, you should be able to add to my branch if you follow the steps in this article https://tighten.com/blog/adding-commits-to-a-pull-request/. You could also do what you suggested and create a new folder and clone my fork of the repo there. Since you don't have the "maintainer" permissions on Altair, I went ahead and invited you to my fork separately, so you should have no issues pushing directly to this branch.

If you run into issues with this, I am also more than happy for you to copy over these changes manually and open a new PR, I don't want you to be held up by git issues

Thanks a lot Joel, that worked! Essentially everything related to git takes two more steps than I expect. In this case, I think I was trying to go directly to the git checkout step, and was missing the git remote add step and was also missing the git fetch step.

Hi @joelostblom, I'm trying to get a sense for the issues you mentioned #2592 (comment)

Does the following count as tab-completion working for an instance? (I didn't make any changes to the code.) I don't really see how this is substantively different from the non-working example alt.X().bin(ma...).

Interesting example, I didn't know that would work. Maybe it has something to do with the function only being called when the cell is executed and before that tab completion can't inspect further? But it must be be possible to bypass that because tab completion is working for eg alt.Chart().<tab> and I think also for alt.Chart().mark_point(col<tab> (on my phone right now and can't double check).

We would ideally want it working for alt.X(). bin(ma<tab> and alt.X().b<tab>.

Hi @joelostblom, I've browsed around a little but haven't made any real progress. I tried using the Python console, and also there alt.X.bi will tab-complete to alt.X.bin but alt.X().bi will not tab-complete. So it's at least not some special functionality related to Jupyter Lab or VS Code.

On the other hand, in Jupyter Lab, alt.Chart().mark_point(si will tab-complete to alt.Chart().mark_point(size=, but alt.Chart().mark_point(si won't tab-complete in the Python console.

One non-Altair example, in Jupyter Lab, pd.DataFrame().max(ax will tab-complete to pd.DataFrame().max(axis=, but pd.DataFrame().max(ax won't tab-complete in the Python console.

I'm not sure what the upshot is to that, just thought I would write it down!

Thanks again for working on this Chris! I believe that the special tab completion functionality you are referring to is inherent to IPython, so if you try in the IPython console rather than the regular one you should see similar completion to that of vs code and juputerlab (although the order of the suggestions might differ).

Hi @joelostblom, thanks for the reply! Just to clarify, the tab-completions in the console I was talking about above are just in the regular Python console (like what I get by typing "Python" into terminal). In that Python console, alt.X.bi will tab-complete, but alt.X().bi won't tab-complete.

I will keep looking around, but please keep me updated on any ideas! I haven't thought about this kind of issue before.

Hi @joelostblom, do you think the tab-completion issues are related to __getattr__ and __dir__ being defined explicitly in https://github.com/altair-viz/altair/blob/bff48d5a235363a74df881c0d167206a9bbb45e1/altair/utils/schemapi.py#L255-L260 and in https://github.com/altair-viz/altair/blob/bff48d5a235363a74df881c0d167206a9bbb45e1/altair/utils/schemapi.py#L474-L475

I tried making a toy example with (as far as I can tell) similar logic and the behavior seemed similar. I put the following in a separate file named sample.py:

class A:
    def __init__(self):
        self.x = 2
        
    def __getattr__(self, attr):
        if attr in ["y", "z"]:
            return 5
    
    def __dir__(self):
        return ["y", "z"]

and then tried tab-completion in Jupyter Lab:

and

To me this feels similar to the behavior we saw above.

In this toy example, if we wanted A(). to have y and z offered as completions, is it clear how we should proceed?

Hmm, one thing that seems different is that in your example A.<tab> does not complete whereas in our original example, we can see that alt.X.bi<tab> does complete. So I am not sure if it is exactly the same, and I am not sure what would be the best way to proceed on the top of my head unfortunately.

Hi @joelostblom,

I haven't had any success getting the auto-completion to work so far.

Do you see any potential in taking a slightly different approach like the following?

I tried adding a few sample methods to the FieldChannelMixin class:

    def set_bin(self, *args, **kwargs):
        copy = self.copy()
        copy.bin = core.BinParams(*args, **kwargs)
        return copy

    def set_scale(self, *args, **kwargs):
        copy = self.copy()
        copy.scale = core.Scale(*args, **kwargs)
        return copy

    def set_title(self, *args, **kwargs):
        copy = self.copy()
        copy.title = core.Text(*args, **kwargs)
        return copy

    def set_legend(self, *args, **kwargs):
        copy = self.copy()
        copy.legend = core.Legend(*args, **kwargs)
        return copy

In my simple tests that seemed to work well. For example, the following code works (based on your code above):

And auto-completion seems to behave better:

and

It seems to me like our lives would be a lot easier if we used a syntax like bin for the attribute and set_bin for the attribute setter method.

If it doesn't make sense to use the same definitions for everything that is a FieldChannelMixin, the definitions could be made more locally.

I also wasn't sure if specifying something like title = core.Text was too restrictive for this use.

Do you have any immediate comments on that type of approach? I wanted to check in before trying to work on it. @jakevdp and @mattijn I'd also be very happy to hear your thoughts.

Hmmm I don't have a good sense of what a solution here might be, but in general I would prefer working with a .bin method rather than a .set_bin method. Jake's older comment here seems to suggest that he is/was of similar opinion:

I suppose set_scale() makes sense, but I find explicit getters & setters like this to be a bit cluttery.

Could a way forward be to break this down to a minimal reproducible example outside of altair and try to troubleshoot it that way? We could even post it to stackoverflow and get some input. I tried this earlier but ran into a separate issue that was specific to JupyterLab and didn't have time to rework the example. I don't know if we can include the specifics of this problem while still making it small enough for SO, but it might still be a useful way forward for us to troubleshoot?

I will also say that personally I believe that even if we don't have tab completion, the fact that the docstring help popup is now working properly makes this PR worth merging, but it would still be very nice to get the tab completion working as well.

@joelostblom can you synchronise this PR with the main repo?
I feel the same, the current setter methods using decorators make more sense over explicit set_ functions. Also agreed that we can work on tab completion in a follow-up PR if we feel this PR is worth merging already. Tests and documentation are still missing I think?

Thanks for the comments! I am happy with whatever approach you think is best. I did come across this comment by Jake from almost exactly one year ago that made me wonder about alternatives to redefining bin, scale, etc as functions.

I'm happy to work on producing a minimal example of the failure of tab-completion, although my initial attempts make me think that whatever we produce will be too complicated and will get downvoted on Stack Overflow ;)

By the way, I don't think the title portion of the very top example above is working, or at least it didn't work on my end when I tried:

import altair as alt
from vega_datasets import data

source = data.cars.url

alt.Chart(source).mark_point().encode(
    alt.X('Horsepower:Q').scale(zero=False),
    alt.Y('Miles_per_Gallon:Q').scale(zero=False).title('Miles / Gallon'),
    alt.Color('Origin:N').title('').legend(orient='bottom').scale(scheme='Set2')
)

can you synchronise this PR with the main repo?

Done! Tests seem to be failing because of an issue installing chromedriver on the test image?

Tests and documentation are still missing I think?

Yup, do you think it would be enough to create a version of each of the gallery examples with this new syntax and use those as tests? Or should we create some new types of tests?

I did come across this #2517 (comment) from almost exactly one year ago that made me wonder about alternatives to redefining bin, scale, etc as functions.

I was thinking about that too, but it does indeed seem like a bigger change. Maybe it is better though because it will make it really clear when we are trying to access and attributes value versus trying to call a function?

By the way, I don't think the title portion of the very top example above is working, or at least it didn't work on my end when I tried

I can confirm that this didn't work for me either. There are other parameters such as stack that don't work either so maybe this is expected? But if I recall correctly it was working before to do .title(), maybe I'm misremembering.

I re-run the tests and these are fine now based on current defined tests.

Basically currently only variables that can be declared as ={} in the encoding channel will function:

foo = alt.Chart().mark_point().encode(
    alt.X(scale={})
).to_dict()

bah = alt.Chart().mark_point().encode(
    alt.X().scale()
).to_dict()

foo == bah  # True

But variables that only can be declared as str will not work:

alt.Chart().mark_point().encode(
    alt.X().shorthand('field:Q')
).to_dict()

Gives:

TypeError: 'UndefinedType' object is not callable

I would suggest to have a test suite that shows which variables work, but also which variables are (as expected) not working.

with pytest.raises(TypeError):
    alt.Chart().mark_point().encode(
        alt.X().shorthand('field:Q')
    ).to_dict()

Maybe it can be done in combination with @pytest.mark.parametrize()

I can understand why this results in a TypeError (not clear error though) but it can be confusing too. Maybe its sensible for shorthand, but for variables such as title and stacked its not strange thinking to define it as the examples that in earlier stages of this PR did seem to work (wrongly?).

Hi @mattijn, I didn't totally understand the str vs ={} distinction you mentioned. Does that explain which of these functions are working and which don't? Do you see what the issue is? Here are the ones that seem to be working/not working for alt.X():

import altair as alt

schema = alt.X().resolve_references()

for k in schema['properties'].keys():
    print(k)
    print(getattr(alt.X(),k))
    print()

produces the following, so I guess bandPosition, impute, etc are not working at the moment.

aggregate
<altair.utils.schemapi._PropertySetter object at 0x163541000>

axis
<altair.utils.schemapi._PropertySetter object at 0x16358cf70>

bandPosition
Undefined

bin
<altair.utils.schemapi._PropertySetter object at 0x16358cc40>

field
<altair.utils.schemapi._PropertySetter object at 0x16358ee90>

impute
Undefined

scale
<altair.utils.schemapi._PropertySetter object at 0x16358ea10>

sort
<altair.utils.schemapi._PropertySetter object at 0x16358f2b0>

stack
Undefined

timeUnit
Undefined

title
Undefined

type
<altair.utils.schemapi._PropertySetter object at 0x1635a87c0>

It seems that currently the attribute setter is enabled only for properties in the encoding channel that can contain an object. Strange though, that impute is Undefined since it clearly is possible to define an object as well for impute in VL: https://vega.github.io/vega-lite/docs/impute.html#encoding.

I'm still a little apprehensive about having something like alt.X().axis evaluate to something different from the method alt.X().axis(). Overall that just seems kind of confusing to me. What would you think about eventually removing this second alt.X().axis value, so that alt.X().axis returns the function that's called when we use alt.X().axis()?

But I'm not at all confident in my opinion and am happy to hear that having both is good and not unusual.

@mattijn Thanks for the information about which encoding channels seem to be using the property setters. I haven't wrapped my head around what's happening, but I should have some time soon to look at it.

Yeah, I agree that it can be confusing. I am not against having something like what is suggested in #2517 (comment). Not sure how much breakage this would cause for people's workflows though. I mostly use chart.to_dict() currently myself because of #2497

I can't get python tools/generate_schema_wrapper.py to work at the moment. It's very possible I'm just confused, but the command python tools/generate_schema_wrapper.py seems to work as expected if I'm on the master branch.

I pushed an update that uses absolute imports, it should work as long as you have an editable install (pip -e) locally. Other options we could consider is to have a startup file (https://stackoverflow.com/a/64310290/2166823 and https://stackoverflow.com/a/57292232/2166823)

I think as things are currently written, python tools/generate_schema_wrapper.py will add the property_setters to v3 and v4. Is that intentional? It seems like it would be better to only add them to v5. (But it's possible I broke something while making edits.)

I think this change should not be backported to older version. I made a change in the schema to reflect this. I think we should probably regenerate the 4.17 vegalite api with the latest versions that actually used 4.17 and then freeze that folder and have all following changes only be present in the v5 folder.

I've been thinking more about the experiments from #2592 (comment)

Does adding something like

axis: axisCallable
bandPosition: bandPositionCallable
bin: binCallable
...

to every class with @with_property_setters seem reasonable? (I don't think this has any impact other than helping auto-completion.) Those would appear in many classes (so there would be a lot of repetition, but then the definitions of axisCallable, etc, would only happen once.

My understanding is this general strategy is endorsed by Jedi: https://jedi.readthedocs.io/en/latest/docs/usage.html?highlight=annotations#type-hinting but it's also very possible I misunderstand, or that there is a simpler solution (or that this won't actually work when we try to implement it).

One thing that seems a little strange to me is, the with_property_setters is happening after the class is defined. For example, it uses cls.resolve_references(). But these lines axis: axisCallable, etc. are getting inserted as part of the class definition. That feels a little asymmetric to me.

The hardest part seems to be deciding which methods we need (like axis, bin, etc.), and it seems wrong to do this work in two different places, once inside of with_property_setters and once in whatever code would write the axis: axisCallable, etc. lines.

@joelostblom and @mattijn, do you see any way to have these axis: axisCallable lines happening as part of the with_property_setters definition?

Not sure if that makes any sense or not!

With documentation I merely meant explanation of this approach within the documentation. How is this different vs current approach. I don't think we should already update all the examples in the example gallery in this PR.

The current codeblock of defining the self.__doc__ in the class _PropertySetter(object) feels a bit.. custom. Not sure if that is very future proof with automatic changes in the API. I also don't see it elsewhere in the altair codebase that the .__doc__ is defined like this.

I still feel still a bit hesitant regarding the non-functioning tab completion for the alt.X().bin structure. @ChristopherDavisUCI did you get one step further with the type hinting?

Edit: I just noticed you post something

Hi @mattijn, good timing! My understanding is that the above approach gets auto-completion working (like alt.X().bi to alt.X().bin and gets the correct arguments to show up (like maxbins inside of bin(). So I think it's mostly a question of whether this is a good approach or not.

So for my understanding, your potential approach will define more classes in the channel.py file. Currently the classes defined in this file are the different Encoding Channels and you are going to extend these with the Encoding Channel Options GitHub page Altair-doc.

These options will be different for different encoding channels so these have to be defined using a kind of lookup in the with_property_setters?

When reading this linked to the GitHub page of these encoding channel options, I noticed that it defines the class altair.PositionFieldDef as being valid for the channels X and Y and when looking to this class here in the core.py file, it seems it has defined all the appropriate encoding channel options.

Can we use these directly instead of defining new ones in the channel.py?

And then there is also no many repetition in the with_property_setters, apart from writing a certain logic that connects the channel to the correct option class:

With documentation I merely meant explanation of this approach within the documentation. How is this different vs current approach.

Yup I will add a section in the docs about this. Before releasing the next version, I think we should have a discussion of whether the method based approach should be the default (which I think sounds reasonable if we don't come across any drawbacks), but for now I will write the docs page as if it isn't.

I don't think we should already update all the examples in the example gallery in this PR.

I think it is a good idea to do all the conversions now just so that we can see that the entire test suite is passing with the new syntax, and we are not overlooking anything. We don't need to merge them in this PR (or could merge them into a separate v5 folder).

The current codeblock of defining the self.doc in the class _PropertySetter(object) feels a bit.. custom. Not sure if that is very future proof with automatic changes in the API. I also don't see it elsewhere in the altair codebase that the .doc is defined like this.

I hear you regarding the custom part, it definitely has a "home-made" feeling to it. But to some extent I think this entire idea of this property setter is quite custom and that type of logic is not available anywhere else in the library either. For the docstring, we are mostly just propagating the properties of the actual class to the property setter wrapper, but in the last commits I also add some custom logic to make those docstrings even more informative, which I think is the part that looks the most home-made right now.

I am open to other solutions here, maybe @ChristopherDavisUCI 's solution for tab completion will take care of this too so that we don't need to fiddle this much with the docstring?

Ah yes. I forgot that the example gallery also functions as test suite.

But the side effect is that the enhancement in this PR now becomes the main approach to specify Altair charts.

But the side effect is that the enhancement in this PR now becomes the main approach to specify Altair charts.

I agree that this is something we should discuss (although I think it is probably a good idea since the new syntax is more convenient to both type and read). We should probably make a new v5 folder for these update examples so that we still keep the old ones for testing the old syntax. I can do that before merging this, I just wanted to make it easy to see what was changed in each example so I am keeping the names the same for now. Later, we can decide if we want to show both the new and old syntax in the gallery in a tabbed interface or keep the old examples in a hidden folder just for testing purposes.

Good. For me personally, I'm in favour of the method based approach if the tab completion works. Let's wait patiently until the succesful findings of Chris. No pressure;).

@ChristopherDavisUCI: I mentioned this before:

When reading this linked to the GitHub page of these encoding channel options, I noticed that it defines the class altair.PositionFieldDef as being valid for the channels X and Y and when looking to this class here in the core.py file, it seems it has defined all the appropriate encoding channel options.

Can we use these directly instead of defining new ones in the channel.py?

But I think I was wrong here isn't it? You need classes of the variables within these classes if I understand correctly?

Thanks @mattijn! It does seem like things might be able to be streamlined a little by adding to the PositionFieldDef definition, for example, but I think the overall difficulty is going to be similar. I'm hoping to get something that works, and then maybe we can look at it and see if there's a way to make it more elegant.

A note that there is a small inconsistency in that this new method based syntax does not apply to the datum function. We can do alt.datum(4, scale=None) but not alt.datum(4).scale(None). This doesn't seem like a big deal to me but thought I would record it since datum can be used instead of the encoding class alt.X() etc so it might be expected that the syntax is similar. Relevant gallery example here https://joelostblom.github.io/altair-docs/gallery/pacman_chart.html

Hi @joelostblom, am I right that it does still work if you use alt.XDatum or alt.ThetaDatum rather than alt.datum? I think maybe alt.datum didn't exist when this PropertySetters code was originally written, so it might be possible to add.

That does work! Then I think we should probably add it to alt.datum too to avoid it being the odd one out which could be confusing.

Hi @joelostblom and @mattijn, I just wanted to say it might be another week before I'm able to get back to looking at this. Just wanted to pass along that quick update.

Hi @joelostblom and @mattijn, do you know what produces the Type column in these docs? I think I want to have access to those same values.

I've stared at this code for a few minutes:
https://github.com/altair-viz/altair/blob/09988e9df19578c398dd24c55dd9b1c00c7d400b/altair/sphinxext/schematable.py#L96
but don't really get the picture of what's happening in there. Do you know if that's the right place to look?

I think I found two approaches. Here for eg the class channel options of alt.PositionFieldDef for channel X and Y:

1. First is to use the type_description from the sphinx extension (this is how the table in the docs is generated):

import altair as alt
from altair.sphinxext.schematable import type_description

schema = alt.PositionFieldDef.resolve_references()

for prop, propschema in schema.get("properties", {}).items():
    print(prop, type_description(propschema))

aggregate :class:`Aggregate`
axis anyOf(:class:`Axis`, `null`)
bandPosition `number`
bin anyOf(`boolean`, :class:`BinParams`, `string`, `null`)
field :class:`Field`
impute anyOf(:class:`ImputeParams`, `null`)
scale anyOf(:class:`Scale`, `null`)
sort :class:`Sort`
stack anyOf(:class:`StackOffset`, `null`, `boolean`)
timeUnit anyOf(:class:`TimeUnit`, :class:`TimeUnitParams`)
title anyOf(:class:`Text`, `null`)
type :class:`StandardType`

2. Another approach is to use the functions within utils part of the tools that are used to generate the altair codebase.

from tools.schemapi import utils
import altair as alt

si = utils.SchemaInfo(alt.PositionFieldDef)
for p in si.properties:
    print(p, si.properties[p].medium_description)

aggregate anyOf(:class:`NonArgAggregateOp`, :class:`ArgmaxDef`, :class:`ArgminDef`)
axis anyOf(:class:`Axis`, None)
bandPosition float
bin anyOf(boolean, :class:`BinParams`, string, None)
field anyOf(:class:`FieldName`, :class:`RepeatRef`)
impute anyOf(:class:`ImputeParams`, None)
scale anyOf(:class:`Scale`, None)
sort anyOf(:class:`SortArray`, :class:`AllSortString`, :class:`EncodingSortField`, :class:`SortByEncoding`, None)
stack anyOf(:class:`StackOffset`, None, boolean)
timeUnit anyOf(:class:`TimeUnit`, :class:`TimeUnitParams`)
title anyOf(:class:`Text`, None)
type enum('quantitative', 'ordinal', 'temporal', 'nominal')

I made a new branch adding some type hints in #2769 (I didn't want to make edits to this branch directly because I am sure there are some issues in what I've produced). I'm very happy for any comments!

Implemented in #2795